Fix off-by-one issue in isOverlapping
#2066
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2066 +/- ##
=======================================
Coverage 49% 49%
- Complexity 6296 6302 +6
=======================================
Files 664 664
Lines 59632 59632
Branches 8648 8648
=======================================
+ Hits 29462 29470 +8
+ Misses 27954 27951 -3
+ Partials 2216 2211 -5 ☔ View full report in Codecov by Sentry. |
|
Excellent fix. Right, offset 0 length 2 means columns 0 to 1 inclusive, so 2 is not in that range and would not overlap. Just logging the thinking here. This fix may have some unforseen impact in code that compensates for the bug at (indirect) call sites. I recommend we test this with rascal-core running with this implementation on the standard library and the big tests at least once before merging. If there are no problems: merge. If there are many, we could fix those simultaneously and then merge. |
|
@PaulKlint has done this by now using the big-test script, which always takes the latest main commit from rascal (unless configured otherwise). a wait, it's not merged yet. we could point the script at this branch. |
|
@sungshik can we add a test for this case? |
|
Tests added! |
|
|
||
| import Location; | ||
|
|
||
| test bool isOverlapping1() = isOverlapping(|unknown:///|(0, 2), |unknown:///|(0, 2)); |
There was a problem hiding this comment.
how about moving this to this already existing file with location tests?
rascal/src/org/rascalmpl/library/lang/rascal/tests/basic/Locations.rsc
Lines 437 to 448 in cbba43c
There was a problem hiding this comment.
Ok, sure, I'll do.
For future reference, though, which tests go in lang::rascal::tests::basic::* and which tests go in lang::rascal::tests::library::*? I was assuming that tests related to language primitives would go in basic::* and tests for library functions in library::*, but I now also notice for instance:
And:
There was a problem hiding this comment.
I have no good answer for this. Maybe @jurgenvinju or @PaulKlint remembers the reason for the split?
In general, I just look at where other functions from the same module are tested.
There was a problem hiding this comment.
I am the first to admit that this organization can (and should) be improved, but the ratio is this:
- tests for languages features go to
basic,functionalityor one of the other more specialized directories - tests for the standard libraries go to
library
As a consequence, tests for, say, locations can be found in two places. Not very good. The problem is that there are several dimensions that can be used to organize the tests:
- syntactic category (e.g., expression, function, pattern, extend, import)
- syntactic alternative (e.g., call, addition, is-defined)
- language feature (e.g., dynamic dispatch, overloading, memoization, parsing)
- data types (e.g., bool, int, map).
- library modules (e.g., Location)
In many cases there is overlap between the categories. Come up with a better organization and I will be happy to implement it.
There was a problem hiding this comment.
Ok, thanks for clarifying!
|
Follow this run to see if it breaks new things: https://github.com/usethesource/rascal-core-big-tests/actions/runs/11720235621 although I would expect the rename code in vs code to be more impacted by changes in this logic. |
|
All the test pass, also the big rascal-core. So merging this. |
Before this PR:
After this PR: